[release-24.0] VDiff: save a sample for every drained extra row so reconciliation can match them (#20855) - #20943
Conversation
…n match them (#20855) Signed-off-by: Matthias Crauwels <matthias.crauwels@planetscale.com> Signed-off-by: Matt Lord <mattalord@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Matt Lord <mattalord@gmail.com>
vitess-bot
Bot
added
the
Type: Enhancement
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Backports the VDiff fix for reconciling drained extra rows without false-positive mismatches.
Changes:
- Samples every drained row up to the configured cap.
- Tracks lossless samples and safely reconciles them.
- Adds drain, resume-error, truncation, and reconciliation tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
workflow_differ.go |
Updates reconciliation and sample trimming. |
workflow_differ_test.go |
Expands reconciliation coverage. |
table_differ.go |
Reworks row draining and progress tracking. |
table_differ_test.go |
Tests draining and stream failures. |
report.go |
Adds lossless-sample metadata. |
report_test.go |
Updates report expectations. |
primitive_executor.go |
Removes the obsolete drain helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if advanceSource { | ||
| lastProcessedRow = sourceRow |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65656c109e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| dr.ExtraRowsSource += drainedRows | ||
| dr.ProcessedRows += drainedRows | ||
| dr.ExtraRowsSourceDiffs = append(dr.ExtraRowsSourceDiffs, drainedDiffs...) |
There was a problem hiding this comment.
When the target is exhausted immediately after an extra-target comparison, sourceRow is still held and the new advanceSource guard leaves lastProcessedRow at the preceding row (or nil), while this block successfully drains that held row and merges its count and sample into dr. If reconciliation, mismatch persistence, or the completed-state update subsequently fails in diffTable, the deferred progress update stores the advanced report without the corresponding source lastpk; resuming then streams those source rows again and double-counts them. Update the checkpoint to the final successfully drained source row atomically with merging drainedRows.
AGENTS.md reference: AGENTS.md:L119-L124
Useful? React with 👍 / 👎.
| // deliberately affirmative: samples that are lossy (only-pks, truncated | ||
| // values) -- or that were persisted by an older binary and reloaded on | ||
| // resume -- lack it and are excluded from reconciliation. | ||
| LosslessValues bool `json:"LosslessValues,omitempty"` |
There was a problem hiding this comment.
This changes user-visible VDiff reconciliation results and also adds LosslessValues to serialized row samples, but the commit contains no release/deployment-note update. Add the required callout so operators and report consumers are informed of the changed behavior and JSON output.
AGENTS.md reference: AGENTS.md:L232-L234
Useful? React with 👍 / 👎.
| var drainedDiffs []*RowDiff | ||
| for targetRow != nil { | ||
| if dr.ExtraRowsTarget+drainedRows < maxExtraRowsToCompare { | ||
| diffRow, err := td.genRowDiff(td.tablePlan.targetQuery, targetRow, reportOpts) |
There was a problem hiding this comment.
When a workflow projection transforms or renames columns, such as select c0 as c1, buildTablePlan preserves c0 as c1 in sourceQuery but emits c1 in targetQuery, and genRowDiff uses those formatted expressions as the Row map keys. Generating drained target samples from targetQuery therefore makes them unequal to otherwise identical source samples during the new reflect.DeepEqual(...Row...) reconciliation, so the drained-row fix still reports false extras for renamed, aggregate, and time-zone-adjusted projections; use a common projection/key representation for both sides.
Useful? React with 👍 / 👎.
| // sample's provenance (the side-specific debug query, the lossless | ||
| // marker which differs between samples persisted by older binaries | ||
| // and current ones), not the row itself. | ||
| if reflect.DeepEqual(dr.ExtraRowsSourceDiffs[i].Row, dr.ExtraRowsTargetDiffs[j].Row) { |
There was a problem hiding this comment.
The Row map is a display representation rather than a lossless encoding: for example, genRowDiff calls sqltypes.Value.ToString(), which represents both SQL NULL and an empty non-binary string as "", while LosslessValues only tracks omitted or truncated columns. With the newly collected samples for every drained row, a later drained row containing NULL can therefore compare equal here to the corresponding source row containing ''; reconciliation subtracts both extras and increments MatchingRows, hiding a real data difference that the old single drained sample would have left reported. Preserve and compare typed values or use an injective canonical encoding instead of the report map.
Useful? React with 👍 / 👎.
Description
This is a backport of #20855